Catcher covered with tests#167

Open

Reversean wants to merge 4 commits intomasterfrom

Member

Reversean commented Feb 25, 2026

Covering with tests components in @hawk.so/javascript:

catcher.ts
utils/validation.ts

Coverage report:

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	62.59	56.2	66.9	62.46
src	96.71	95.09	91.89	96.66
catcher.ts	96.66	95.09	91.66	96.62	158,188,252-255,337
errors.ts	100	100	100	100
src/addons	53.48	35.07	60	53.52
breadcrumbs.ts	47.94	34.88	47.61	47.94	247,279,317-366,375,386-441,451,473-476,480-514,523,531-535,552-556,564-568,575,589-605,641
consoleCatcher.ts	65.21	35.41	78.57	65.67	66,88,102-137,187,198-211,237,285-287
src/integrations	0	0	0	0
vue.ts	0	0	0	0	43-250
src/modules	56.21	49.47	60.86	56.21
fetchTimer.ts	63.63	100	50	63.63	24-25,30-32
sanitizer.ts	57.62	65.85	94.11	57.62	53,62,69,76,83,115-123,139,146,176,232-241,251,267-295
socket.ts	32.65	11.53	18.75	32.65	88-118,142-143,153,157-209
stackParser.ts	76	60.71	77.77	76	50,62,71-72,85-93,134,151-154
src/utils	75	79.03	90.9	74.6
event.ts	88.88	100	100	88.88	44
id.ts	100	100	100	100
log.ts	80	80	100	80	24,39
selector.ts	0	0	0	0	10-43
validation.ts	100	100	100	100

Reversean added 2 commits

February 25, 2026 20:23


          test(catcher): Catcher covered with tests

ffff7d6


          test(catcher): Validation utils covered with tests

90d2fb6

Reversean mentioned this pull request

UserManager abstraction added #158

Open

Base automatically changed from chore/core-workspace to master

March 3, 2026 16:41

neSpecc requested a review from Copilot

March 3, 2026 16:43

Copilot started reviewing on behalf of neSpecc

March 3, 2026 16:43

Copilot AI reviewed

View reviewed changes

Copilot AI left a comment

Pull request overview

Adds Vitest coverage support and introduces new unit tests to increase runtime coverage for the JavaScript SDK’s Catcher and validation utilities.

Changes:

Added comprehensive tests for Catcher behavior (delivery, dedupe, context/user handling, global handlers, addons).
Added focused tests for src/utils/validation.ts helpers.
Added a test:coverage script and the @vitest/coverage-v8 dependency to enable coverage reporting.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File	Description
yarn.lock	Locks new coverage-related dependencies (notably `@vitest/coverage-v8`).
packages/javascript/package.json	Adds `test:coverage` and `@vitest/coverage-v8` dev dependency.
packages/javascript/tests/utils/validation.test.ts	New tests for validation helpers.
packages/javascript/tests/catcher.test.ts	New tests covering `Catcher` end-to-end behavior using a mocked transport.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/javascript/tests/utils/validation.test.ts Outdated

+              import { validateUser, validateContext, isValidEventPayload, isValidBreadcrumb } from '../../src/utils/validation';
+              // Suppress console output produced by log() calls inside validation failures.
+              vi.mock('../src/utils/log', () => ({ default: vi.fn() }));

Copilot AI Mar 3, 2026

vi.mock path is incorrect from tests/utils/validation.test.ts. validation.ts imports log from ./log (resolves to src/utils/log.ts), but this test is mocking ../src/utils/log (which would resolve under tests/src/...). Update the mock to target the actual module (e.g. ../../src/utils/log).

Suggested change

      
            vi.mock('../src/utils/log', () => ({ default: vi.fn() }));
          
            vi.mock('../../src/utils/log', () => ({ default: vi.fn() }));

Copilot uses AI. Check for mistakes.

Member Author

Reversean Mar 3, 2026

Fixed.

packages/javascript/tests/catcher.test.ts Outdated

Comment on lines +348 to +352

+                  it('should accept a plain string as a shorthand for the token', async () => {
+                    const sendSpy = vi.fn().mockResolvedValue(undefined);
+                    const transport: Transport = { send: sendSpy };
+                    const hawk = new Catcher({

Copilot AI Mar 3, 2026

Test name doesn't match what's being exercised: this case constructs new Catcher({ token: TEST_TOKEN, ... }) (settings object), not new Catcher(TEST_TOKEN) (string shorthand). Rename the test (or change the setup) so the description matches the behavior under test.

Copilot uses AI. Check for mistakes.

Member Author

Reversean Mar 3, 2026

Fixed.

packages/javascript/tests/catcher.test.ts Outdated

Comment on lines +513 to +517

+                  it('should log a warning when transport.send rejects', async () => {
+                    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+                    const transport: Transport = {
+                      send: vi.fn().mockRejectedValue(new Error('network error')),
+                    };

Copilot AI Mar 3, 2026

The description says "log a warning", but the code under test logs with type 'error' (and this test spies on console.error). Rename the test to reflect the actual behavior.

Copilot uses AI. Check for mistakes.

Member Author

Reversean Mar 3, 2026

All cases that checked log effect have been redone: now they check that catcher handles inner Errors.

neSpecc reviewed

View reviewed changes

packages/javascript/tests/catcher.test.ts Outdated

+                // The Catcher's primary responsibility: capture errors and forward them to
+                // the configured transport with identifying metadata.
+                describe('error delivery', () => {
+                  it('should deliver an Error instance via transport with matching title', async () => {

Member

neSpecc Mar 3, 2026

what does it mean "with matching title"?

Member Author

Reversean Mar 3, 2026

Meaning we expect that HawkJavaScriptEvent.title same as send Error.message

Member

neSpecc Mar 3, 2026

this statement is not clear. Make a test case name more straightforward.
What to you want to test? "error should be sent" or "payload title should be composed from error message"?

Member Author

Reversean Mar 3, 2026

Changed test names.

packages/javascript/tests/catcher.test.ts Outdated Show resolved Hide resolved

packages/javascript/tests/catcher.test.ts Outdated Show resolved Hide resolved

packages/javascript/tests/catcher.test.ts Outdated Show resolved Hide resolved

packages/javascript/tests/catcher.test.ts Outdated

Comment on lines +142 to +152

+                  it('should ignore setUser when called with null', async () => {
+                    const { sendSpy, transport } = createTransport();
+                    const hawk = createCatcher(transport);
+                    hawk.setUser({ id: 'valid' });
+                    hawk.setUser(null as never);
+                    hawk.send(new Error('e'));
+                    await wait();
+                    expect(getLastPayload(sendSpy).user?.id).toBe('valid');
+                  });

Member

neSpecc Mar 3, 2026

seems redundant

Member Author

Reversean Mar 3, 2026

Removed.

packages/javascript/tests/catcher.test.ts Outdated

Comment on lines +384 to +392

+                  it('should construct without error when given a bare token string', () => {
+                    const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+                    const addELSpy = vi.spyOn(window, 'addEventListener').mockImplementation(() => undefined);
+                    expect(() => new Catcher(TEST_TOKEN)).not.toThrow();
+                    addELSpy.mockRestore();
+                    consoleSpy.mockRestore();
+                  });

Member

neSpecc Mar 3, 2026

duplicates "should accept a plain string as a shorthand for the token"

Member Author

Reversean Mar 3, 2026

In general, it would be better to call this case this way.

Fixed.

packages/javascript/tests/catcher.test.ts Outdated

+                });
+                // ── Transport failure ─────────────────────────────────────────────────────
+                describe('transport failure', () => {

Member

neSpecc Mar 3, 2026

move to a separate file

Member Author

Reversean Mar 3, 2026

Moved to catcher.transport.test.ts.

packages/javascript/tests/catcher.test.ts Outdated

+                // ── Transport failure ─────────────────────────────────────────────────────
+                describe('transport failure', () => {
+                  it('should log a warning when transport.send rejects', async () => {

Member

neSpecc Mar 3, 2026

the case is not clear

Member Author

Reversean Mar 3, 2026

This test checks that in this case we will leave an error log (in fact the only effect in this case)

Member Author

Reversean Mar 3, 2026

Redone case by checking if call does not throw.

packages/javascript/tests/catcher.test.ts Outdated

+                    errorSpy.mockRestore();
+                  });
+                  it('should log a warning when beforeSend throws unexpectedly', async () => {

Member

neSpecc Mar 3, 2026

the case is not clear. Why it throws?

Member Author

Reversean Mar 3, 2026

Why it throws?

Since we receive beforeSend from outside, it may probably throw an error.

This test checks that in this case we will leave an error log (in fact the only effect in this case)

Member Author

Reversean Mar 3, 2026

Redone case by checking if call does not throw.

packages/javascript/tests/catcher.test.ts Outdated

+                // ── Environment addons ────────────────────────────────────────────────────
+                //
+                // Browser-specific data collected from window (URL, GET params, debug info).
+                describe('environment addons', () => {

Member

neSpecc Mar 3, 2026

move to a separate file, like "browser-addons.test.ts"

Member Author

Reversean Mar 3, 2026

Moved to catcher.addons.test.ts.

Reversean added 2 commits

March 4, 2026 00:37


          test(catcher): reduced duplicating tests

6aebdda


          test(catcher): sections from catcher.test.ts separated on different t…

fbd1089

…est.ts files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet